Fix broken array patching and ensure all values in draft Maps/Sets are finalized#1201
Merged
mweststrate merged 7 commits intomainfrom Dec 28, 2025
Merged
Conversation
Somehow was seeing an error with `each` "not existing" on startup when running the prod build tests
Pull Request Test Coverage Report for Build 20547109483Details
💛 - Coveralls |
Collaborator
Curious, was this minified to a |
mweststrate
reviewed
Dec 28, 2025
|
|
||
| function getPath(state: ImmerState, path: PatchPath = []): PatchPath | null { | ||
| // Step 1: Check if state has a stored key | ||
| if ("key_" in state && state.key_ !== undefined) { |
Collaborator
There was a problem hiding this comment.
ugh, I should have spotted this in review
mweststrate
approved these changes
Dec 28, 2025
Collaborator
|
Thanks for following up on these! |
Collaborator
Author
|
@mweststrate I see a |
Collaborator
|
Ok, scrambled the setup a bit more and triggered a new 11.1.2 release, that one should contain all changes now. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR:
getPathbeing broken in our pre-minified CJS/ESM production builds (but not the mainimmer.mjsartifact, which is minified at app build time)MaporSetand never gets finalized properlyvitest.build.config.tsto ensure that it does actually run against a real minified production artifactarrayTrapsto usefor..infor setup instead ofeach(), to work around an odd issue witheach()somehow not existing asproxy.tsis being executed in the production buildpackage.jsonto only rebuild the library whentest:buildis executed, as the previous use of"pretest": "yarn build"meant that it didn't get run if you were specifically executingyarn test:build.Fixes #1199
Fixes #1200
Background
After shipping RTK 2.11 with Immer 11, , a user reported an issue with RTKQ optimistic updates not working right when updating a nested array. They also said this only happened in prod builds:
After a bunch of discussion, we finally got a repro of that issue, and the repro showed that the patch contents had a very different path calculated when it failed.
Meanwhile, another user reported what they thought was a related problem, but turned out to be completely different. This involved updating nested
Maps andSets.Fixes
After investigation, I found that:
key_, but the logic for patch path lookup had aif ("key_" in state)check. That check failed whenkey_got renamed.DraftSetinstance, puts it in a new plain object, and puts the plain object into aDraftMap. That effectively cuts the chain of assignments, and we never tried to fix up theDraftSetin the plain object. We already have ahandleCrossReferencemethod that does this, it just wasn't being called inside ofDraftMap.set()orDraftSet.add()(and also needed to be updated to handle working withSets correctly)Note that this meant that patches worked for most apps, if the build system picked up the normal
immer.mjsartifact (which is unminified, has embeddedNODE_ENVvalues, and will get minified by the app's build step). The problem was if the build system picked our our pre-minified artifacts (immer.cjs.production.jsorimmer.production.mjs), where ESBuild had already mangled the field names. Turns out that Expo does this, at least some of the time.In the process of working on this, I also found that my attempt to port the "production build" tests to Vitest wasn't working right - it wasn't actually loading the production artifact, so the tests were just running against the source code still. I was able to fix that problem by updating the
aliassettings to be more specific.While working on that, I also tweaked the
test:buildscript to actually contain the build command.Finally, while running the prod tests, I saw this very surprising error:
I actually still have no explanation for why this is happening. Looking at the prod artifacts, I see a minified version of
each(), it exists in scope, and it ought to work fine. But whatever, changing this to be afor..inloop works and passes tests.